feat: the languagecontainer module to share data and functions with language containers#2489
feat: the languagecontainer module to share data and functions with language containers#2489
Conversation
The language containers will use the containers module.
fbdb3dd to
94d4477
Compare
de4b626 to
98a96c1
Compare
98a96c1 to
3e0d668
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2489 +/- ##
==========================================
+ Coverage 85.62% 86.39% +0.76%
==========================================
Files 102 102
Lines 10403 10587 +184
==========================================
+ Hits 8908 9147 +239
+ Misses 1168 1122 -46
+ Partials 327 318 -9 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Test passed. |
|
@codyoss Would you review package structure in this new "containers" module? |
|
@meltsufin As talked yesterday, for each module in google-cloud-go 's librariangen, I put the reasons for include/exclude them in the pull request description. |
codyoss
left a comment
There was a problem hiding this comment.
Overall I like the idea of this package but I think it would be useful to have a design of the public API.
|
|
||
| go 1.25.0 | ||
|
|
||
| require github.com/googleapis/librarian v0.0.0 |
There was a problem hiding this comment.
I think we should try to avoid depending on the other module if possibile.
There was a problem hiding this comment.
With the comment you wrote for the "message" package, I'm now thinking of removing this module and having the language containers to depend on the github.com/googleapis/librarian module. The "languagecontainer/*" are packages inside the module that they use for language-container specific work.
There was a problem hiding this comment.
It's now in go/sdk-librarian-containers-in-go?tab=t.ft3dwbukyu66
|
|
||
| require github.com/googleapis/librarian v0.0.0 | ||
|
|
||
| replace github.com/googleapis/librarian => ../ |
There was a problem hiding this comment.
I would avoid putting replace statement in a module other ppl will depend on. It can create cases where things will build here locally with CI but fail for ppl that depend on the library.
There was a problem hiding this comment.
I'm now thinking of using the root librarian module for sharing with language containers. PTAL go/sdk-librarian-containers-in-go?tab=t.ft3dwbukyu66
| // Package bazel provides functions to parse Bazel-related files. | ||
| package bazel | ||
|
|
||
| // TODO: Implement this package. Reference https://github.com/googleapis/google-cloud-go/tree/main/internal/librariangen/bazel |
There was a problem hiding this comment.
nit: don't add an empty package. We can add later when we know what we want this to look like.
| // generate-request.json and generate-response.json. | ||
| // https://github.com/googleapis/librarian/blob/main/doc/language-onboarding.md#generate | ||
| type GenerateFlags struct { | ||
| Librarian string |
There was a problem hiding this comment.
nit: add godoc to an public struct fields.
There was a problem hiding this comment.
Let me focus on the package organization for now.
|
|
||
| // LanguageContainerFunctions lists the functions a language container | ||
| // defines to accept requests from Librarian CLI. | ||
| type LanguageContainerFunctions struct { |
There was a problem hiding this comment.
nit: I would just call this LanguageContainer
There was a problem hiding this comment.
and or move all of these things up a directory and call Functions. That would give usage the look: languagecontainer.Functions in calling code.
There was a problem hiding this comment.
That's a great idea. I added that in go/sdk-librarian-containers-in-go?tab=t.ft3dwbukyu66 as languagecontainer.Functions.
| // } | ||
| // | ||
| // ``` | ||
| func LanguageContainerMain(args []string, functions LanguageContainerFunctions) { |
There was a problem hiding this comment.
nit: Run. If you like the other feedback about that would make it languagecontainer.Run(...)
There was a problem hiding this comment.
Yes, I took that idea in go/sdk-librarian-containers-in-go?tab=t.ft3dwbukyu66.
| cmd := exec.CommandContext(ctx, args[0], args[1:]...) | ||
| cmd.Env = os.Environ() | ||
| cmd.Dir = workingDir | ||
| slog.Debug("librariangen: running command", "command", strings.Join(cmd.Args, " "), "dir", cmd.Dir) |
There was a problem hiding this comment.
nit: remove all references to librariangen here. This is an idiom usually only used in error msgs and signals where an error came from. For logging you can see the source line if that feature is turned on. ( It is off by default)
There was a problem hiding this comment.
Let's forget about this execv package for now to focus on the package organization.
| // See the License for the specific language governing permissions and | ||
| // limitations under the License. | ||
|
|
||
| package execv |
There was a problem hiding this comment.
I think the API for such a package should just mimic https://pkg.go.dev/os/exec#Cmd. It would be nice it was a drop in replacement for the stdlib -- API wise.
|
|
||
| // Package message declares the definition of the messages which Librarian | ||
| // CLI and the language containers read and write. | ||
| package message |
There was a problem hiding this comment.
If we do create a package like this I almost think a good goal here would be to invert the dep. The CLI should depend on this package. Although it is allowed since this in in the same monorepo, an outside package could never do what you did here by reaching into a Go internal directory.
There was a problem hiding this comment.
I put the idea of creating a "message" package, outside the "internal" package. go/sdk-librarian-containers-in-go?tab=t.ft3dwbukyu66
| // limitations under the License. | ||
|
|
||
| // Package protoc provides functions to run Protobuf compiler (protoc command) | ||
| package protoc |
There was a problem hiding this comment.
Same a bazel, don't add it until define.
|
Let me write a note about the plan. |
suztomo
left a comment
There was a problem hiding this comment.
@codyoss @quartzmo PTAL go/sdk-librarian-containers-in-go?tab=t.ft3dwbukyu66. I'll close this pull request for now.
|
|
||
| require github.com/googleapis/librarian v0.0.0 | ||
|
|
||
| replace github.com/googleapis/librarian => ../ |
There was a problem hiding this comment.
I'm now thinking of using the root librarian module for sharing with language containers. PTAL go/sdk-librarian-containers-in-go?tab=t.ft3dwbukyu66
|
|
||
| go 1.25.0 | ||
|
|
||
| require github.com/googleapis/librarian v0.0.0 |
There was a problem hiding this comment.
It's now in go/sdk-librarian-containers-in-go?tab=t.ft3dwbukyu66
| cmd := exec.CommandContext(ctx, args[0], args[1:]...) | ||
| cmd.Env = os.Environ() | ||
| cmd.Dir = workingDir | ||
| slog.Debug("librariangen: running command", "command", strings.Join(cmd.Args, " "), "dir", cmd.Dir) |
There was a problem hiding this comment.
Let's forget about this execv package for now to focus on the package organization.
| // } | ||
| // | ||
| // ``` | ||
| func LanguageContainerMain(args []string, functions LanguageContainerFunctions) { |
There was a problem hiding this comment.
Yes, I took that idea in go/sdk-librarian-containers-in-go?tab=t.ft3dwbukyu66.
|
|
||
| // LanguageContainerFunctions lists the functions a language container | ||
| // defines to accept requests from Librarian CLI. | ||
| type LanguageContainerFunctions struct { |
There was a problem hiding this comment.
That's a great idea. I added that in go/sdk-librarian-containers-in-go?tab=t.ft3dwbukyu66 as languagecontainer.Functions.
|
|
||
| // Package cmd provides the main function LanguageContainerMain that | ||
| // handles command line argument parsing and invocation of the corresponding methods. | ||
| package cmd |
There was a problem hiding this comment.
Yes, let's do that. I like languagecontainer.Run idea. go/sdk-librarian-containers-in-go?tab=t.ft3dwbukyu66
| // generate-request.json and generate-response.json. | ||
| // https://github.com/googleapis/librarian/blob/main/doc/language-onboarding.md#generate | ||
| type GenerateFlags struct { | ||
| Librarian string |
There was a problem hiding this comment.
Let me focus on the package organization for now.
|
|
||
| // Package message declares the definition of the messages which Librarian | ||
| // CLI and the language containers read and write. | ||
| package message |
There was a problem hiding this comment.
I put the idea of creating a "message" package, outside the "internal" package. go/sdk-librarian-containers-in-go?tab=t.ft3dwbukyu66
The languagecontainer module and packages
This change introduces
github.com/googleapis/librarian/languagecontainermodule which consists of the following packages:bazel: the functions for Bazel-file parsingcmd: the entry point function that parses command line argumentsmessage: the data definition of the messages between the CLI and the language container (using the existing ones). Thereplace github.com/googleapis/librarian => ../line incontainers/go.modallows us to reuse the data definition outside thecontainersfolder.execv: the execve child process executionprotoc: the protoc command wrapper(Cody shared me that in Go, we should avoid using a generic package name such as "util". https://go.dev/blog/package-names#package-names says "They are often simple nouns". The package names above follow the guidelines.)
The module name "languagecontainer" is from the convention (no word separator) I found in the modules in https://github.com/googleapis/google-cloud-go. It's descriptive than the original idea of "containers" (plural).
They are mostly from the packages from https://github.com/googleapis/google-cloud-go/tree/main/internal/librariangen. I renamed
requesttomessage(because it includes the response). I didn't includepostprocessor(the logic is specific to google-cloud-go),testdata(the data is specific to google-cloud-go),build,generate, andrelease(they are the implementation of Go language containers commands).Language Container's View
A language container declares the dependency as below:
where
cloud.google.com/go/internal/librarian/java_containeris language container's package name (irrelevant to this PR) and06b8d3305e95a7b9efe3e6641d45488e421dca21is a SHA of https://github.com/googleapis/librarian repository. Thego mod tidycommand replaces the SHA with the data + SHA (e.g.,v0.0.0-20251007191759-06b8d3305e95).With
cmd.LanguageContainerMain, the main function of a language container will look like this:Tests
I added a test case as cmd_test.go to demonstrate that the argument parsing tests.
Because we test argument parsing in this module, the language container implementations will not need to test their argument parsing.